Skip to content

Mods as feature + Patches implementation#257

Open
patchzyy wants to merge 20 commits intodevfrom
Patches-Converter
Open

Mods as feature + Patches implementation#257
patchzyy wants to merge 20 commits intodevfrom
Patches-Converter

Conversation

@patchzyy
Copy link
Copy Markdown
Member

@patchzyy patchzyy commented Apr 28, 2026

Purpose of this PR:

Add a patches conversion workflow so installed mods with incompatible full archive overrides can be converted into patches, making mod stacking safer and reducing launch-time conflicts.

How to Test:

  • Build and run WheelWizard.
  • Import or install a mod containing convertible .szs files or revo_kart.brsar.
  • Open the Patches page and confirm incompatible mods show the warning/convert action.
  • Convert the mod to patches and verify the original archive is replaced with generated loose patch files.
  • Enable multiple patch mods with different priorities, launch Retro Rewind/RR Beta, and confirm the active Riivolution Patches folder is prepared using priority order.

What Has Been Changed:

  • Added SZS/Yaz0/U8 archive decoding and patch extraction against embedded Mario Kart Wii baseline data.
  • Added BRSAR patch conversion for supported RBNK/RSEQ/RWSD entries and loose BRSAR patch filename normalization.
  • Added embedded baseline resources and kart SZS allow-list data used to match modded files to clean game files.
  • Reworked mod management into feature services for installation, persistence, priority ordering, and launch preparation.
  • Updated the Patches page UI with incompatibility warnings, convert-to-patches actions, and priority controls.
  • Updated launchers to prepare enabled mods into the correct Riivolution patch folders before launch.
  • Added helpers for big-endian/binary string parsing, directory/file handling, and operation-result convenience.

Summary by CodeRabbit

  • New Features

    • Added patch conversion workflow for incompatible mods with compatibility warnings
    • Added archive decoding and extraction support
    • Rebranded mods system to "Patches"
  • Bug Fixes & Improvements

    • Improved error handling and reporting for launcher operations
    • Enhanced mod installation validation and cleanup
    • Added detailed logging for unhandled exceptions

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 28, 2026

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 4d80f03f-f8fd-4efe-b809-e901c477901e

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch Patches-Converter

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 28, 2026

Caution

Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted.

Error details
{}

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 28

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
WheelWizard/Shared/OperationResult/OperationResult.cs (1)

78-89: 🧹 Nitpick | 🔵 Trivial

Avoid logging every handled failure at this shared layer.

These helpers now emit an error log for every caught exception, but the PR also surfaces OperationError through the UI path where failures are logged again. That will double-report routine handled errors and inflate error telemetry. Consider logging only at the outer boundary, or making this helper's logging opt-in.

Also applies to: 112-123, 141-152, 174-185

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@WheelWizard/Shared/OperationResult/OperationResult.cs` around lines 78 - 89,
The helper is logging every caught exception inside OperationResult.TryCatch
which causes double-reporting; change the TryCatch implementations (the catch
blocks around OperationResult.TryCatch and its overloads) to make logging opt-in
by adding an optional parameter (e.g., bool logOnCatch = false) to the TryCatch
methods and only call Log.Error when that flag is true; update all affected
overloads (the catch blocks that construct OperationError with
Message/Exception/MessageTranslation/TitleReplacements/ExtraReplacements) to
respect the new flag and keep the default behavior as non-logging so outer
boundaries can log as needed.
WheelWizard/Views/Layout.axaml.cs (1)

105-135: 🧹 Nitpick | 🔵 Trivial

Remove the temporary no-op PropertyChanged wiring until it is re-enabled.

The subscription at Line 105 currently points to an effectively empty handler, so it adds lifecycle complexity without behavior. Consider deleting both until UpdateModsActionIndicator() is reintroduced.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@WheelWizard/Views/Layout.axaml.cs` around lines 105 - 135, Remove the
temporary no-op PropertyChanged wiring by deleting the subscription line
"ModManagerService.PropertyChanged += ModManager_PropertyChanged;", its
corresponding unsubscription "ModManagerService.PropertyChanged -=
ModManager_PropertyChanged;" in OnClosed, and the empty handler method
"ModManager_PropertyChanged(...)" (including its commented TODO) until
UpdateModsActionIndicator is reintroduced; keep references to
UpdateModsActionIndicator in comments if desired but remove the live hookup and
teardown to avoid lifecycle complexity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@WheelWizard/Features/Archives/ISzsArchiveDecoder.cs`:
- Line 3: The top-of-file comment is stale (it refers to a “sealed record”) but
the file actually declares the ISzsArchiveDecoder interface; update or remove
that comment so it correctly describes this file (e.g., change to “Interface for
SZS archive decoding” or delete the comment) and ensure the symbol
ISzsArchiveDecoder remains the subject of the comment.

In `@WheelWizard/Features/Archives/SzsArchiveDecoder.cs`:
- Around line 9-15: Guard against truncated buffers before reading fixed-width
headers: in TryDecodeU8Archive (and the other similar entry points that call
BigEndianBinaryHelper.BufferToUint32 or ReadAscii) check raw.Length is at least
the number of bytes you will read (e.g., 4 bytes for a magic, plus any bytes
needed for a root-offset/read) immediately after DecompressYaz0IfNeeded and
before calling BigEndianBinaryHelper.BufferToUint32 or ReadAscii; if the buffer
is too short, throw an InvalidDataException with a clear message instead of
letting an IndexOutOfRangeException propagate. Ensure the same pattern is
applied to any methods that parse U8 headers/root offsets (refer to
ParseU8Archive, BigEndianBinaryHelper.BufferToUint32 and any ReadAscii usage) so
all fixed-width reads have explicit length guards.

In `@WheelWizard/Features/Mods/ModInstallationService.cs`:
- Around line 137-145: The zip-slip check in ModInstallationService is
bypassable; compute fullRoot = Path.GetFullPath(destinationDirectory) and ensure
it ends with a directory separator (use Path.EndsInDirectorySeparator or append
Path.DirectorySeparatorChar), then compute fullEntry =
Path.GetFullPath(entryDestinationPath) and verify
fullEntry.StartsWith(fullRootWithSeparator, StringComparison.OrdinalIgnoreCase)
before writing; if the check fails, return Fail(...) as before. Reference:
entryDestinationPath, destinationDirectory, and Fail in ModInstallationService.

In `@WheelWizard/Features/Mods/ModManager.cs`:
- Around line 510-513: The current validation in ModManager.cs uses StartsWith
on raw paths which allows prefix bypass; instead normalize and fully resolve
both paths (use Path.GetFullPath on modDirectory as well), ensure
normalizedModsFolder has a trailing directory separator (append
Path.DirectorySeparatorChar if missing), then verify the resolved modDirectory
either equals normalizedModsFolder or starts with normalizedModsFolder +
Path.DirectorySeparatorChar using StringComparison.OrdinalIgnoreCase; if the
check fails call Fail("Invalid mod directory.").
- Around line 140-147: Remove lingering PropertyChanged subscriptions from mods
when removing them: before removing a Mod in RemoveModAsync, unsubscribe the
event handler you attached when the mod was added (e.g., mod.PropertyChanged -=
OnModPropertyChanged or mod.PropertyChanged -= Mod_PropertyChanged), then remove
it from Mods, call OnPropertyChanged(nameof(Mods)), and proceed to await
SaveModsAsync(); ensure the unsubscribe uses the exact handler method used
during subscription.
- Around line 533-543: QueueSaveMods currently starts independent
fire-and-forget saves causing overlapping writes; fix by serializing saves with
a private SemaphoreSlim (e.g. add a field _saveSemaphore = new
SemaphoreSlim(1,1)) and change QueueSaveMods to call a single enqueuing method
(e.g. _ = EnqueueSaveAsync()) that awaits _saveSemaphore.WaitAsync(), calls
SaveModsAndLogAsync(), and finally calls _saveSemaphore.Release() in a finally
block; keep SaveModsAndLogAsync and SaveModsAsync unchanged except ensure
EnqueueSaveAsync handles exceptions so saves never leave the semaphore locked.

In `@WheelWizard/Features/Mods/ModsLaunchService.cs`:
- Around line 70-74: The current ShouldAskToClearTargetFolder method only checks
for files (Directory.EnumerateFiles) and will miss folders-only cases; change
the existence check to detect any filesystem entries (files or subdirectories)
e.g. use Directory.EnumerateFileSystemEntries(targetFolderPath).Any() or
Directory.GetFileSystemEntries to treat subdirectories as non-empty, keeping the
rest of the predicate (modManager.Mods.Any(mod => mod.IsEnabled) and
Directory.Exists) intact.
- Around line 21-68: The PrepareModsForLaunch method can throw before returning
and may never close the ProgressWindow; wrap the main flow in a
try/catch/finally so that: create and Show the ProgressWindow before running
risky operations, run directory enumeration and the Task.Run(CopyFinalFiles...)
inside the try, catch any Exception and convert it into an OperationResult
failure (returning or assigning a failed OperationResult that includes the
exception message), and always call progressWindow.Close() in finally; ensure
the method returns an OperationResult in all code paths (both success from
CopyFinalFiles and failures from the catch).

In `@WheelWizard/Features/Patches/KartSzsAllowList.cs`:
- Around line 15-20: resourceName lookup currently uses First(...) which throws
InvalidOperationException when the embedded resource is missing; change the
lookup to use FirstOrDefault on assembly.GetManifestResourceNames() (or
otherwise check for a matching name) and if the result is null/empty throw the
existing FileNotFoundException before calling GetManifestResourceStream. Update
the code paths around resourceName, assembly.GetManifestResourceNames(), and
GetManifestResourceStream(...) to perform the null check and throw
FileNotFoundException deterministically.

In `@WheelWizard/Features/Patches/ModPatchConversionService.cs`:
- Around line 126-130: The loop writing conversion.Analysis.Entries assumes the
destination parent folders exist; before calling File.WriteAllBytes(destination,
entry.Bytes) create the directory for Path.GetDirectoryName(destination) (e.g.,
using Directory.CreateDirectory) and handle possible null/empty parent path,
then proceed to write and increment writtenPatchCount; update the block that
computes destination (using Path.Combine(Path.GetDirectoryName(file)!,
entry.ExportPath)) to ensure its parent directory is created first.
- Around line 13-15: The user-facing constant IncompatibleMessage in
ModPatchConversionService.cs contains awkward grammar and should be rewritten;
replace the existing string value for the public const string
IncompatibleMessage with a clear, concise message such as: "This mod is not
compatible with other mods. Launching the game with it installed may cause
visual issues or crashes. Consider converting this mod to a patch by
right-clicking." Ensure the updated string preserves punctuation and
capitalization and is used wherever IncompatibleMessage is referenced.

In `@WheelWizard/Helpers/BigEndianBinaryHelper.cs`:
- Around line 13-19: The bounds-check can overflow when computing offset + N;
update BufferToInt32's guard to use the subtraction form (check offset < 0 ||
offset > data.Length - 4) so large offsets can't wrap and bypass the check, and
apply the same pattern in ResolveDataRef where it currently checks offset + 8 —
replace with offset < 0 || offset > data.Length - 8 (or equivalent subtraction
check) to prevent reads past the buffer; adjust both methods (BufferToInt32 and
ResolveDataRef) to use these safe comparisons and keep the existing exception on
failure.

In `@WheelWizard/Helpers/BinaryStringHelper.cs`:
- Around line 9-23: Validate inputs in both ReadAscii and
ReadNullTerminatedAscii: check bytes is not null, ensure offset is >= 0 and <=
bytes.Length, and ensure length is >= 0 and offset + length <= bytes.Length in
ReadAscii before calling Ascii.GetString; in ReadNullTerminatedAscii ensure
offset is >= 0 and < bytes.Length (return empty string if offset ==
bytes.Length) and then scan for the terminator, protecting against malformed
data so the while/substring logic never reads out of bounds; update the methods
ReadAscii and ReadNullTerminatedAscii accordingly.

In `@WheelWizard/Helpers/FileHelper.cs`:
- Around line 206-209: EnsureDirectory now returns OperationResult but callers
like MoveDirectoryContents still ignore it, causing silent failures; update
MoveDirectoryContents to call EnsureDirectory(path) and check its returned
OperationResult, and if it indicates failure, propagate or return that failure
(or convert to an appropriate OperationResult error) instead of proceeding with
the no-op branch—locate EnsureDirectory and MoveDirectoryContents to add the
check and early-return/propagation when EnsureDirectory reports a failure.

In `@WheelWizard/Models/Mods/Mod.cs`:
- Around line 135-136: The TODO about Mod handling its own INI persistence is
ambiguous—decide ownership and act: either remove the TODO and keep/clarify
SaveToIniAsync on the Mod class (document its responsibility and implement it
fully), or extract INI serialization out of Mod by removing or marking
Mod.SaveToIniAsync obsolete and creating a dedicated serializer (e.g.,
ModIniSerializer with SaveModAsync/LoadModAsync or extension methods) that takes
a Mod instance and the iniFilePath; update all callers to use the new serializer
and add small unit tests for serialization to ensure behavior is preserved.

In `@WheelWizard/Services/Launcher/GoogleLauncher.cs`:
- Around line 17-34: The Launch, Install and Update methods currently always
return Ok() and set installed before awaiting dialog results, which hides
failures; update these methods (Launch, Install, Update) to catch exceptions
from ViewUtils.OpenLink, MessageBoxWindow.ShowDialog (and any async operations),
await the dialog before mutating installed, and return a failure OperationResult
when an exception or non-success outcome occurs (use the existing
OperationResult failure constructor or helper instead of Ok()); ensure installed
is only set after ShowDialog completes successfully and propagate error
messages/details into the failure result so callers can react to real errors.

In `@WheelWizard/Services/Launcher/RrBetaLauncher.cs`:
- Around line 70-95: Install and Update currently let exceptions bubble up while
Launch returns an OperationError; update Install and Update to mirror Launch's
contract by wrapping the await
_customDistributionSingletonService.RetroRewindBeta.InstallAsync(progressWindow)
and UpdateAsync(progressWindow) calls in try/catch blocks that catch Exception
and return an OperationResult representing failure (e.g., OperationError) with
the exception message/details, ensuring both methods still show/close the
ProgressWindow and return an OperationResult on error instead of throwing.

In `@WheelWizard/Services/Launcher/RrLauncher.cs`:
- Around line 36-55: The game-path existence check (PathManager.GameFilePath)
must be performed at the start of Launch() before any state-changing operations;
move the File.Exists check so it runs before DolphinLaunchHelper.KillDolphin(),
the _modsLaunchService.ShouldAskToClearTargetFolder(...) prompt/YesNoWindow, and
the call to _modsLaunchService.PrepareModsForLaunch(...); if the path is missing
return Fail(...) immediately, otherwise proceed with killing Dolphin and
preparing/clearing the Patches folder as currently implemented.

In `@WheelWizard/Services/PathManager.cs`:
- Around line 502-504: Update the misleading comment in PathManager (class
PathManager) so it accurately describes the new Patches flow: replace or reword
the line that mentions syncing into the "loose-mod runtime folder" to clearly
state that the mods folder is synced into the active LooseMods runtime folder
while enabled mods are synced into the active Patches runtime folder (ensure
both targets are named consistently with the codebase: LooseMods runtime folder
and Patches runtime folder).

In `@WheelWizard/Services/Storage/CustomFilePickerFileType.cs`:
- Around line 7-13: The FilePickerFileType All uses a wildcard pattern which
permits arbitrary files; change the picker to only allow expected mod archive
extensions (e.g., update the FilePickerFileType All Patterns,
AppleUniformTypeIdentifiers and MimeTypes to only include .zip, .7z, .rar)
and/or add a post-selection content validation step in the import flow to
inspect the selected file (verify extension and check archive signature/magic
bytes and that extracted entries meet allowed types) and reject with a clear
error; locate the static FilePickerFileType All declaration in
CustomFilePickerFileType and either tighten its
Patterns/MimeTypes/AppleUniformTypeIdentifiers or call a new validation helper
(e.g., ValidateModArchive(filePath)) after file selection to enforce the checks.

In `@WheelWizard/Shared/MessageTranslations/MessageTranslationHelper.cs`:
- Around line 200-224: The current LogOperationError method re-logs
OperationError.Exception with Log.Error causing duplicate stack traces; change
LogOperationError to avoid logging the exception object a second time — e.g.
remove the Log.Error branch and always use Log.Warning (or log only the
exception.Message string) so ShowMessage/AwaitMessageAsync (which call
LogOperationError) won't duplicate the original exception; update references in
LogOperationError and ensure you still include OperationError.Message and, if
needed, Exception.Message but do not pass the Exception object to the logger.

In `@WheelWizard/Views/App.axaml.cs`:
- Around line 120-137: In OpenGameBananaModWindowAsync, avoid calling
IModManager.ReloadAsync when there is no launch protocol; first call
GetLaunchProtocolArgument() and if it's null/whitespace return false, then call
Services.GetRequiredService<IModManager>().ReloadAsync() and handle/log
reloadResult as currently done, finally call
UrlProtocolManager.ShowPopupForLaunchUrlAsync(protocolArgument); this gates the
reload behind presence of the protocol argument and prevents duplicate startup
work.

In `@WheelWizard/Views/Layout.axaml`:
- Around line 128-130: The sidebar label for ModsButton currently hard-codes
"Patches" which breaks localization; update the ModsButton SidebarRadioButton
Text attribute to use the app's localization resource instead (e.g. replace
Text="Patches" with a localized resource reference such as Text="{StaticResource
Patches}" or the project's equivalent dynamic binding to the localization
dictionary), and ensure the corresponding "Patches" key exists in the
localization resources so SidebarRadioButton (ModsButton) displays localized
text.

In `@WheelWizard/Views/Pages/ModsPage.axaml`:
- Around line 107-109: Replace the hardcoded Header="Convert to patches"
MenuItem labels with localized resource bindings: update the MenuItem elements
(the ones tied to Click="ConvertToPatches_Click" and the same occurrences around
the second block) to reference the appropriate language resource key (e.g., a
new/ existing ConvertToPatches entry) instead of literal text, and add that key
to your language resource files; ensure the XAML uses your project's
localization pattern (StaticResource/DynamicResource or x:Static to the res
class) so the MenuItem Header is pulled from resources.

In `@WheelWizard/Views/Pages/ModsPage.axaml.cs`:
- Around line 234-249: The success path currently only shows Skipped via
BuildPatchConversionResultMessage and always uses
MessageBoxWindow.MessageType.Message; instead include conversion.Warnings
alongside conversion.Skipped when building the summary (use
ModPatchConversionResult.Warnings and ModPatchConversionResult.Skipped) and, if
any warnings exist, switch the MessageBoxWindow to MessageType.Warning (rather
than MessageType.Message) so the dialog reflects caveats; apply the same change
to the other success-handling block that also uses
BuildPatchConversionResultMessage and MessageBoxWindow.

In `@WheelWizard/Views/Patterns/SidebarRadioButton.axaml`:
- Around line 82-92: Remove the dead commented PathIcon block or replace it with
a compiled, feature-gated element: either delete the commented <PathIcon>
entirely, or restore it as a real element bound to the template properties
(e.g., <PathIcon Data="{StaticResource WarningTriangle}"
IsVisible="{TemplateBinding WarningVisible}" ToolTip.Tip="{TemplateBinding
WarningTip}" ToolTip.Placement="Right" Width="17" Height="17" Margin="0,0,12,0"
Foreground="{StaticResource Danger500}" HorizontalAlignment="Right"
VerticalAlignment="Center" />) and ensure the control exposes the WarningVisible
and WarningTip properties (or add them) so the icon is compiled but hidden by
default; this prevents commented drift while keeping the feature gated by the
WarningVisible/WarningTip symbols.

In `@WheelWizard/Views/Patterns/SidebarRadioButton.axaml.cs`:
- Around line 56-76: Remove the large commented-out property block from
SidebarRadioButton (the WarningVisibleProperty, WarningVisible,
WarningTipProperty, and WarningTip symbols) to avoid retaining dead code;
alternatively, if the warning API is needed, implement the two StyledProperty
registrations and their WarningVisible and WarningTip accessors in
SidebarRadioButton with safe defaults (e.g., false and empty string) and keep
any UI bindings or template usage disabled until patches stabilize; reference
the SidebarRadioButton class and the exact property names above when making the
change and either delete the commented block or replace it with the minimal,
safe property implementations.

In `@WheelWizard/Views/Popups/ModManagement/ModContent.axaml.cs`:
- Around line 194-205: DownloadAndInstallCurrentModAsync currently returns a
binary Result that conflates true install failures, user cancellation, missing
download, and post-install cleanup warnings; update it to return a richer
InstallOutcome (e.g., enum {Installed, Canceled, NoDownload, CleanupWarning,
Failed}) or an InstallResult DTO containing Outcome and optional Error, and
adjust Install_Click to branch on that Outcome: only show the success MessageBox
when Outcome == Installed, silently ignore or show a canceled notice for
Canceled/NoDownload, show a success + warning message when Outcome ==
CleanupWarning (do not treat it as IsFailure), and show a hard error dialog/log
when Outcome == Failed using installResult.Error. Ensure
MessageTranslationHelper.ShowMessage and the existing success MessageBoxWindow
are called from Install_Click based on the new Outcome values, and that
DownloadAndInstallCurrentModAsync maps cleanup errors to CleanupWarning instead
of returning a failure when the install itself succeeded.

---

Outside diff comments:
In `@WheelWizard/Shared/OperationResult/OperationResult.cs`:
- Around line 78-89: The helper is logging every caught exception inside
OperationResult.TryCatch which causes double-reporting; change the TryCatch
implementations (the catch blocks around OperationResult.TryCatch and its
overloads) to make logging opt-in by adding an optional parameter (e.g., bool
logOnCatch = false) to the TryCatch methods and only call Log.Error when that
flag is true; update all affected overloads (the catch blocks that construct
OperationError with
Message/Exception/MessageTranslation/TitleReplacements/ExtraReplacements) to
respect the new flag and keep the default behavior as non-logging so outer
boundaries can log as needed.

In `@WheelWizard/Views/Layout.axaml.cs`:
- Around line 105-135: Remove the temporary no-op PropertyChanged wiring by
deleting the subscription line "ModManagerService.PropertyChanged +=
ModManager_PropertyChanged;", its corresponding unsubscription
"ModManagerService.PropertyChanged -= ModManager_PropertyChanged;" in OnClosed,
and the empty handler method "ModManager_PropertyChanged(...)" (including its
commented TODO) until UpdateModsActionIndicator is reintroduced; keep references
to UpdateModsActionIndicator in comments if desired but remove the live hookup
and teardown to avoid lifecycle complexity.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: eb738a61-96c1-4442-a9f6-bbfc5b6fc678

📥 Commits

Reviewing files that changed from the base of the PR and between 9949825 and f89bd7d.

📒 Files selected for processing (54)
  • WheelWizard/Features/Archives/ArchivesExtensions.cs
  • WheelWizard/Features/Archives/Domain/DecodedArchive.cs
  • WheelWizard/Features/Archives/Domain/U8Node.cs
  • WheelWizard/Features/Archives/ISzsArchiveDecoder.cs
  • WheelWizard/Features/Archives/SzsArchiveDecoder.cs
  • WheelWizard/Features/GameBanana/Domain/GameBananaModPreview.cs
  • WheelWizard/Features/Mods/ModInstallationService.cs
  • WheelWizard/Features/Mods/ModManager.cs
  • WheelWizard/Features/Mods/ModsExtensions.cs
  • WheelWizard/Features/Mods/ModsLaunchService.cs
  • WheelWizard/Features/Patches/BrsarPatchConverter.cs
  • WheelWizard/Features/Patches/Domain/GameBaselineModels.cs
  • WheelWizard/Features/Patches/GameBaselineStore.cs
  • WheelWizard/Features/Patches/ISzsPatchConverter.cs
  • WheelWizard/Features/Patches/KartSzsAllowList.cs
  • WheelWizard/Features/Patches/LooseBrsarPatchFileName.cs
  • WheelWizard/Features/Patches/ModPatchConversionService.cs
  • WheelWizard/Features/Patches/PatchConversionHelpers.cs
  • WheelWizard/Features/Patches/PatchesExtensions.cs
  • WheelWizard/Features/Patches/Resources/allowed-kart-szs.json
  • WheelWizard/Features/Patches/Resources/game-baseline-data.json
  • WheelWizard/Features/Patches/Resources/game-baseline-index.json
  • WheelWizard/Features/Patches/SzsPatchConverter.cs
  • WheelWizard/Helpers/BigEndianBinaryHelper.cs
  • WheelWizard/Helpers/BinaryStringHelper.cs
  • WheelWizard/Helpers/FileHelper.cs
  • WheelWizard/Models/Mods/Mod.cs
  • WheelWizard/Program.cs
  • WheelWizard/Services/Installation/ModInstallation.cs
  • WheelWizard/Services/Launcher/GoogleLauncher.cs
  • WheelWizard/Services/Launcher/Helpers/ModsLaunchHelper.cs
  • WheelWizard/Services/Launcher/Helpers/RetroRewindLaunchHelper.cs
  • WheelWizard/Services/Launcher/ILauncher.cs
  • WheelWizard/Services/Launcher/RrBetaLauncher.cs
  • WheelWizard/Services/Launcher/RrLauncher.cs
  • WheelWizard/Services/ModManager.cs
  • WheelWizard/Services/ModStorageSystem.cs
  • WheelWizard/Services/PathManager.cs
  • WheelWizard/Services/Storage/CustomFilePickerFileType.cs
  • WheelWizard/SetupExtensions.cs
  • WheelWizard/Shared/MessageTranslations/MessageTranslationHelper.cs
  • WheelWizard/Shared/OperationResult/OperationResult.cs
  • WheelWizard/Views/App.axaml.cs
  • WheelWizard/Views/Layout.axaml
  • WheelWizard/Views/Layout.axaml.cs
  • WheelWizard/Views/Pages/HomePage.axaml.cs
  • WheelWizard/Views/Pages/ModsPage.axaml
  • WheelWizard/Views/Pages/ModsPage.axaml.cs
  • WheelWizard/Views/Pages/TestingPage.axaml.cs
  • WheelWizard/Views/Patterns/GridModPanel.axaml
  • WheelWizard/Views/Patterns/SidebarRadioButton.axaml
  • WheelWizard/Views/Patterns/SidebarRadioButton.axaml.cs
  • WheelWizard/Views/Popups/ModManagement/ModContent.axaml.cs
  • WheelWizard/WheelWizard.csproj
💤 Files with no reviewable changes (4)
  • WheelWizard/Services/Launcher/Helpers/ModsLaunchHelper.cs
  • WheelWizard/Services/Installation/ModInstallation.cs
  • WheelWizard/Services/ModManager.cs
  • WheelWizard/Services/ModStorageSystem.cs

Comment thread WheelWizard/Features/Archives/ISzsArchiveDecoder.cs Outdated
Comment thread WheelWizard/Features/Archives/SzsArchiveDecoder.cs Outdated
Comment thread WheelWizard/Features/Mods/ModInstallationService.cs Outdated
Comment thread WheelWizard/Features/Mods/ModManager.cs
Comment thread WheelWizard/Features/Mods/ModManager.cs Outdated
Comment thread WheelWizard/Views/Pages/ModsPage.axaml Outdated
Comment thread WheelWizard/Views/Pages/ModsPage.axaml.cs
Comment thread WheelWizard/Views/Patterns/SidebarRadioButton.axaml
Comment thread WheelWizard/Views/Patterns/SidebarRadioButton.axaml.cs
Comment thread WheelWizard/Views/Popups/ModManagement/ModContent.axaml.cs
@patchzyy patchzyy changed the title Patches converter Mods as feature + Patches implementation Apr 28, 2026
Copy link
Copy Markdown

@GABRlEL GABRlEL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few questions I had in mind after reading the code and not testing it:
for WheelWizard/Features/Archives/SzsArchiveDecoder.cs:
Does the decompression functionality for SZS support different compression types or even no compression? As an example iirc RR uses different compression than other software and MKWii still reads uncompressed SZS files fine - so this would be something worth looking into to reinforce the functionality.
Also idk if this applies here but is the SZS decompression protected against those directory backtravel attacks (the ./././appdata or smth like that) stuff? Or does that not apply here?

for WheelWizard/Features/Mods/ModInstallationService.cs:
because *.ini is mentioned, does this survive if a desktop.ini is placed in the directory because a user decides to put an icon or similar on the folder?
no tar.gz support? is that not relevant?

for WheelWizard/Features/Mods/ModManager.cs:
consider letting the user know what invalid characters are in the mod name

for WheelWizard/Resources/Languages/Phrases.resx:
general suggestion/wish: would it be possible to let the user know, that if they don't choose to clear their patches or mystuff folder, they do launch the game with active mystuff and patches

Generally check for outstanding todos since there's still some in the comments
Apart from this, and take that with a grain of salt since I'm not a coding pro, lgtm

@patchzyy
Copy link
Copy Markdown
Member Author

patchzyy commented Apr 28, 2026

Just a few questions I had in mind after reading the code and not testing it: for WheelWizard/Features/Archives/SzsArchiveDecoder.cs: Does the decompression functionality for SZS support different compression types or even no compression? As an example iirc RR uses different compression than other software and MKWii still reads uncompressed SZS files fine - so this would be something worth looking into to reinforce the functionality. Also idk if this applies here but is the SZS decompression protected against those directory backtravel attacks (the ./././appdata or smth like that) stuff? Or does that not apply here?

for WheelWizard/Features/Mods/ModInstallationService.cs: because *.ini is mentioned, does this survive if a desktop.ini is placed in the directory because a user decides to put an icon or similar on the folder? no tar.gz support? is that not relevant?

for WheelWizard/Features/Mods/ModManager.cs: consider letting the user know what invalid characters are in the mod name

for WheelWizard/Resources/Languages/Phrases.resx: general suggestion/wish: would it be possible to let the user know, that if they don't choose to clear their patches or mystuff folder, they do launch the game with active mystuff and patches

Generally check for outstanding todos since there's still some in the comments Apart from this, and take that with a grain of salt since I'm not a coding pro, lgtm

1 RR files should never pass this .szs decompression, but it only supports yaz0 and uncompressed SZS files right now, i could look into other compressions but i dont know which ones are out there

2 Backtravel attacks are being accountend for, eg

            if (
                relativePath == "."
                || relativePath == ".."
                || relativePath.StartsWith(".." + Path.DirectorySeparatorChar)
                || relativePath.StartsWith(".." + Path.AltDirectorySeparatorChar)
                || Path.IsPathRooted(relativePath)
            )
            {
                return Fail("Invalid mod directory.");
            }

3 Every .ini file gets scanned on their own, a desktop.ini will not produce a valid output and will be skipped, the other ini files will get parsed correctly

        foreach (var iniFile in iniFilesResult.Value)
        {
            var modResult = await LoadModFromIniAsync(iniFile);
            if (modResult.IsFailure)
                return modResult.Error;

            if (!string.IsNullOrWhiteSpace(modResult.Value.Title))
                mods.Add(modResult.Value);
        }

4 This would mean i'd have to write valid localisation for that :( too lazy

5 fixed

And as for the todo's, I know about them, i consider them out of scope, this feature already got really really out of hand

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants